Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(passport): custom domain #2330

Merged
merged 1 commit into from
Jun 1, 2023
Merged

feat(passport): custom domain #2330

merged 1 commit into from
Jun 1, 2023

Conversation

szkl
Copy link
Contributor

@szkl szkl commented May 25, 2023

Description

Introduces a utility function named getCookieDomain. If the request host is
not matching the defined HOST then the request will be checked for a custom
domain if the request has clientId in the host metadata.

The server entry point implements a similar process to check the custom domain
status at the beginning of the request lifecycle.

Related Issues

Testing

  • Manual

  • Create a custom domain in the console for a published application

  • Do the validation steps and finally create the CNAME record

  • Visit passport using the custom domain and login

Checklist

  • I have read the CONTRIBUTING guidelines
  • I have tested my code (manually and/or automated if applicable)
  • I have updated the documentation (if necessary)

@szkl szkl marked this pull request as draft May 25, 2023 22:09
@szkl szkl self-assigned this May 25, 2023
@szkl szkl added the enhancement Indicates new feature requests label May 25, 2023
@szkl szkl mentioned this pull request May 25, 2023
Closed
5 tasks
@szkl szkl marked this pull request as ready for review May 25, 2023 22:39
@szkl szkl force-pushed the feat/passport/custom-domain branch 2 times, most recently from 417fe41 to 4138f38 Compare May 26, 2023 13:26
@szkl
Copy link
Contributor Author

szkl commented May 26, 2023

Should the cookie SameSite attribute be Strict for custom domains?

betimshahini
betimshahini reviewed May 29, 2023
View reviewed changes
apps/passport/server.ts
`Completed HTTP handler for ${reqURL.pathname}/${reqURL.searchParams}`,
newTraceSpan.toString()
)
const app = await starbaseClient.getAppPublicProps.query({ clientId })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a thought, not sure if we should do this but worth putting out there: if we're to set a acceptedDomain metadata prop when we store the clientId on there, we'd be able to check both things without having to call starbase at all. I'm thinking the headers are injected more quickly in the request compared to the roundtrip time of passport->starbase->DO and back. Headers would get bigger is one downside, and we don't know the consistency/reliability characteristics of the metadata store. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could do that but it'd be a redundant check. The request host and the host in the metadata would be the same assuming Cloudflare wouldn't make a mistake. I think it is okay to try it out during beta.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other condition checked is the status of the domain which depends on the validations. It can be ignored only as long as Cloudflare passes traffic over the custom hostname.

betimshahini
betimshahini reviewed May 29, 2023
View reviewed changes
apps/passport/wrangler.toml Outdated
@@ -54,10 +55,11 @@ services = [
]

[env.dev.vars]
HOST = "passport-dev.rollup.id"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call this DEFAULT_HOST or PASSPORT_HOST or something, for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@betimshahini
Copy link
Contributor

Should the cookie SameSite attribute be Strict for custom domains?

I think some of the callback redirects won't work with Strict. IIRC that's the reason why kept it as it currently is for non-custom domain passport.

@betimshahini
Copy link
Contributor

@szkl As discussed, this PR should prevent access to /settings route and its children when accessed through custom domain. This will be revisited later under #2337

@szkl szkl force-pushed the feat/passport/custom-domain branch 3 times, most recently from dc0a68c to c58de35 Compare May 29, 2023 19:14
@szkl
Copy link
Contributor Author

szkl commented May 30, 2023

@szkl As discussed, this PR should prevent access to /settings route and its children when accessed through custom domain. This will be revisited later under #2337

I found out that a request to /authorize is needed to complete the authentication for an application. Should Passport make that request if the initial request is for a custom domain? Should the application redirect to the passport authorize endpoint on the custom domain?

@szkl szkl force-pushed the feat/passport/custom-domain branch 3 times, most recently from e589e37 to 322b887 Compare May 30, 2023 14:05
@szkl szkl force-pushed the feat/passport/custom-domain branch from 322b887 to 93be7a7 Compare May 30, 2023 14:40
@szkl
Copy link
Contributor Author

szkl commented May 30, 2023

Pending a test on the development environment.

@szkl szkl force-pushed the feat/passport/custom-domain branch 4 times, most recently from 844725e to 56974f5 Compare May 31, 2023 09:56
@szkl szkl requested a review from betimshahini May 31, 2023 11:45
@szkl szkl force-pushed the feat/passport/custom-domain branch from 56974f5 to dcd9c23 Compare May 31, 2023 15:02
@szkl szkl force-pushed the feat/passport/custom-domain branch from dcd9c23 to 6483f8e Compare May 31, 2023 18:57
@betimshahini betimshahini merged commit f3dd8b9 into main Jun 1, 2023
@betimshahini betimshahini deleted the feat/passport/custom-domain branch June 1, 2023 07:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betimshahini betimshahini betimshahini approved these changes

@maurerbot maurerbot Awaiting requested review from maurerbot

Assignees

@szkl szkl

Labels
enhancement Indicates new feature requests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

chore(passport): Implement Custom domain support
2 participants
@szkl @betimshahini